Skip to content

feat(relayenv): re-anchor mechanism#720

Open
aaron-zeisler wants to merge 20 commits into
feat/concurrent-keysfrom
aaronz/SDK-2542/re-anchor-mechanism
Open

feat(relayenv): re-anchor mechanism#720
aaron-zeisler wants to merge 20 commits into
feat/concurrent-keysfrom
aaronz/SDK-2542/re-anchor-mechanism

Conversation

@aaron-zeisler

@aaron-zeisler aaron-zeisler commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements T2.c — the re-anchor mechanism (SDK-2542) for concurrent SDK keys. When sdkKey.value changes, relay now swaps its single upstream SDK client to the new anchor synchronously, preserving downstream connections and rolling back cleanly on failure.

Jira: SDK-2542 · Epic: SDK-2453 · Milestone M3.

Targets feat/concurrent-keys (not v8). Builds on the T0 PoC findings and the store-handover spike.

Background

The design (.agent-docs/concurrent-keys/phase1-design.md §7) and the T0 PoC established that re-anchoring is not a transparent side-effect of today's code: flipping the anchor pointer before the new client initializes leaves GetClient() nil mid-swap (PoC H6) and strands the env on init failure (H7); the in-memory store is rebuilt empty when the new client constructs (H1/H5). This PR closes those gaps.

Changes (one commit per logical step)

  1. test(store) — bring over the three store-handover real-client spike tests (one rewritten as TestRealClient_HandoverPreservesUnderlyingStore).
  2. feat(store)SSERelayDataStoreAdapter.Build reuses its existing wrapper on a second client construction (store handover → no empty-store window). The wrapper is refcounted: only the final Close tears down the underlying store, so the retiring client's Close can't pull the store out from under the new anchor (design §7 lifecycle caveat).
  3. feat(credential)Reconcile no longer flips the anchor; it returns a ReconcileResult (AnchorChange + MobilePrimaryRepoint), and a new CommitAnchor moves the pointer. The new anchor is stripped from additions in Case A so addCredential's async startSDKClient can't race the synchronous build.
  4. feat(relayenv) — synchronous re-anchor with Case A (build new client + store handover + wait Initialized → commit → ReplaceCredential → big-segment stub; rollback on init failure) and Case B (reuse the existing client → commit → ReplaceCredential). Order: add → re-anchor → remove, serialized by a dedicated reconcileMu.
  5. test(relayenv) — integration tests: Case A success, Case A init-failure rollback, Case B reuse (no new client), mobile-primary repoint.
  6. docs — document Case B in design §7.

Acceptance criteria coverage

Criterion Where
Case A success (build, init, flip, store handover, old client retained) TestReanchorSync_CaseA_BuildsNewClientAndMovesAnchor
Case A init failure → rollback (anchor stays, old client serves, structured error) TestReanchorSync_CaseA_InitFailureRollsBack
Case B reuse (no new Build, anchor flips) TestReanchorSync_CaseB_ReusesExistingClient
Store handover (Build reuse; Close doesn't tear down shared store) TestRealClient_HandoverPreservesUnderlyingStore, TestReanchorPoC_H5_StoreSurvivesReAnchor, H1 sub-test
Mobile primary switch repoints event dispatcher; non-primary does not TestReanchorSync_MobilePrimaryRepoint_AlreadyAcceptedKey + PR #712's gate
No orphan clients (Case A doesn't trip #716 guard; Case B spawns nothing) Case A / Case B tests

PoC tests for H1 and H5 were inverted from "asserts broken behavior" to "asserts the fix" and retained as regressions.

Out of scope

  • T2.d (SDK-2543) — big-segment sync re-wire. A clean call site (reanchorBigSegmentSync) is defined as a no-op (matches today's behavior, no regression); T2.d fills it in.
  • T2.e — handler fan-out.

Post-merge note (catch-up onto feat/concurrent-keys)

The base moved 11 commits ahead after this branch was cut, including the sdkKey→anchor rename (#722), the legacy single-key rotation path removal (#723), the SDKKeyParams/MobileKeyParams builder + AcceptedKeys() reshaping (#729/#730), and the RAC/offline wiring to ReconcileCredentials (#719). This branch now catch-up merges feat/concurrent-keys and re-applies the T2.c intent on top of the refactored API:

  • Reconcile still defers the anchor flip to CommitAnchor; the in-place flip that the rebased reconcileSDKKeys introduced was removed. Case A still strips the new anchor from additions.
  • The base unit tests that assumed Reconcile flips in place (TestAcceptedKeys, TestReconcileAlreadyExpiredKeyIsIgnoredOnAdd) were updated to the deferred-flip contract (CommitAnchor to observe the anchor).
  • PoC H6/H7 were inverted to assert the fix (no nil swap-window; rollback keeps the old anchor authoritative), completing the same treatment H1/H5 received.
  • feat(credentials): wire RAC and offline handlers to ReconcileCredentials #719 is now merged. Its ReconcileCredentials-based regression TestReAnchoringToKeyStillInGraceReusesItsClient and this PR's TestReanchorSync_CaseB_ReusesExistingClient both cover Case B reuse; they are complementary (the former focuses on no-leak/close-of-revoked-key, the latter on no-new-build + anchor flip) and are both retained.

Review fix — re-anchor rollback no longer poisons initErr

A review of the merge caught a regression in the rollback path: on a failed re-anchor to an invalid new key the SDK returns ErrInitializationFailed, and reanchor was storing that in c.initErr. The request middleware treats GetInitError() == ErrInitializationFailed as an env-level auth failure and returns 401 for every request — which would have rejected all traffic to an environment that was still healthy on the previous anchor, defeating the rollback. Fixed: initErr is left untouched on rollback (the env is healthy on the old anchor); the failure still surfaces via the structured Errorf log. The Case A init-failure and H7 tests now assert GetInitError() stays nil and that the failure is logged.

Review fix — serialize the credential cleanup ticker against re-anchor

A second review finding (MEDIUM): the cleanup ticker path (cleanupExpiredCredentialstriggerCredentialChanges) drained the rotator's StepTime queue and ran addCredential/removeCredential without holding reconcileMu, so a credential expiry firing during an in-flight synchronous re-anchor ran concurrently with the reanchor sequence. Both paths drain the same StepTime queue (the ticker could steal additions a reconcile just queued) and a ticker removeCredential could close a client mid-reanchor. The synchronous re-anchor widened this window versus the pre-T2.c base. Fixed: triggerCredentialChanges now takes reconcileMu for its whole StepTime + add/remove pass, serializing the ticker against reconcileCredentials the same way concurrent reconciles already are (no re-entrancy — reconcileCredentials never calls that path). New regression TestReanchorSync_CredentialExpiryDuringReanchorIsSerialized wedges a re-anchor open mid-build and asserts the ticker blocks until it releases.

Review fix (LOW) — re-anchor peripheral install branches on one signal

reanchor now gates peripheral install (envStreams / handler / connection-mapper wiring) solely on change.NewAnchorPreviouslyAccepted rather than on the Case A/B c.clients[newAnchor] != nil branch. The two signals always agree today (a client can only exist for a previously-accepted key, since removeCredential deletes the client in lockstep with the rotator dropping the key), but branching peripheral setup on the rotator signal means a brand-new anchor is never stranded without routing even if that invariant were ever broken.


Note

High Risk
Changes core credential reconciliation, upstream SDK client lifecycle, and shared data-store refcounting during anchor rotation—failure or concurrency bugs could break serving or auth for whole environments.

Overview
Implements synchronous SDK anchor re-anchoring when sdkKey.value changes: the upstream client swap no longer flips the anchor during Reconcile or races async startSDKClient.

Credential rotator now returns ReconcileResult (AnchorChange, MobilePrimaryRepoint) and defers the anchor move to CommitAnchor; brand-new anchors are stripped from additions so the env owns client build. RevertAnchorChange realigns the accepted set on Case A rollback.

reconcileCredentials runs add → synchronous reanchor → remove under reconcileMu (also held by the cleanup ticker’s triggerCredentialChanges). reanchor builds or reuses the new anchor client before commit, calls ReplaceCredential on events/metrics, rolls back without poisoning initErr, and handles mobile-primary repoint when the new primary was already accepted.

Store handover: SSERelayDataStoreAdapter.Build reuses the existing wrapper with refcounted Close so the retiring client does not tear down shared in-memory data.

Design §7 documents Case A vs Case B; PoC tests assert fixed behavior; new integration and real-client tests cover success, rollback, reuse, serialization, and offline commit-without-build. Big-segment re-wire remains a stub (T2.d).

Reviewed by Cursor Bugbot for commit 2fc4f20. Bugbot is set up for automated code reviews on this repo. Configure here.

Three tests (TestRealClient_CloseInvokesWrapperClose,
TestRealClient_ReadsAfterCloseAreStillFunctional,
TestRealClient_HandoverScenarioToday) exercise the real ld.LDClient
against SSERelayDataStoreAdapter / streamUpdatesStoreWrapper.

The T0 PoC could not validate the wrapper's Close behavior against
the real client (it used a fake). These confirm the design's
lifecycle caveat: ld.LDClient.Close() propagates through the wrapper
to the underlying store. With store handover (next commit), the
adapter — not the retiring client's wrapper — must own that
lifecycle.
SSERelayDataStoreAdapter.Build now hands its existing wrapper to a
second SDK client (re-anchor handover) instead of constructing a
fresh wrapper around a fresh underlying store. The new anchor's
client therefore reads populated, initialized data immediately — no
empty-store window during re-anchor.

The wrapper is refcounted: each handover bumps the count via
acquire(); each client's Close decrements; only the final Close
tears down the underlying store. This honors design §7's
store-lifecycle caveat — Close must not tear down the shared store
while another client still holds it — without leaking the store on
final env teardown.

Two PoC tests that asserted the old broken behavior (H1 in-memory
sub-test, H5 in-memory wipe) are inverted to assert the post-fix
invariant. The spike test TestRealClient_HandoverScenarioToday is
renamed and rewritten to TestRealClient_HandoverPreservesUnderlyingStore
covering the full refcounted lifecycle.
…mitAnchor

Adds the API surface for the synchronous re-anchor sequence (design §7).

- Reconcile no longer flips r.primarySdkKey when the anchor changes.
  Callers receive a ReconcileResult carrying an AnchorChange struct
  (PreviousAnchor, NewAnchor, NewAnchorPreviouslyAccepted) and drive
  the flip via the new CommitAnchor(key) method once they are ready.
- ReconcileResult also exposes MobilePrimaryRepoint when the primary
  mobile key changed to a key that was already in the accepted set.
  In that case the key is not in additions and addCredential's gate
  won't fire, so the caller must invoke eventDispatcher.ReplaceCredential
  itself.
- env_context's reconcileCredentials applies the anchor change
  immediately after Reconcile to preserve pre-T2.c behavior. The full
  synchronous sequence (build new client first, then commit) lands in
  the next commit. The MobilePrimaryRepoint case is handled now —
  this is the fix for the gap left by PR #712's gate (already-accepted
  mobile key becoming primary skipped event-dispatcher repointing).

Two rotator tests that previously relied on Reconcile flipping the
anchor in place now call CommitAnchor explicitly with the returned
ReconcileResult.AnchorChange.
Implements the synchronous re-anchor sequence described in design §7.
reconcileCredentials now interleaves add → re-anchor → remove and owns
the new anchor's setup, replacing the prior commit's immediate
CommitAnchor.

The rotator strips the new anchor from additions in Case A so the
async startSDKClient invocation in addCredential cannot race the
synchronous client build. reanchor branches on whether a client
already exists for the new anchor:

  Case A (no client): install peripherals if the key is brand new
  (envStreams, handlers, connection mapping); construct the SDK
  client synchronously via c.sdkClientFactory with store handover
  (Build returns the existing wrapper, so the new client reads
  populated initialized data); on err or !Initialized, roll back —
  do not CommitAnchor, log a structured error, leave the previous
  anchor authoritative; on success, install the client, rebuild the
  evaluator, then CommitAnchor + ReplaceCredential + big-segment stub.

  Case B (client exists, e.g. a former anchor in grace): reuse the
  client and skip the build; CommitAnchor + ReplaceCredential +
  big-segment stub.

A new reconcileMu serializes concurrent reconciles without blocking
other env operations (GetClient / GetStore / addCredential continue
to run during the synchronous build).

Rotator-test updates: tests that previously expected the anchor in
the additions list now expect it stripped, and explicitly invoke
CommitAnchor mirroring what env_context's reconcileCredentials does.

T2.d (SDK-2543) will fill in reanchorBigSegmentSync; until then it is
a no-op (matching today's behavior — no regression on re-anchor).
Regression coverage for the synchronous re-anchor sequence (design §7):

- Case A success: re-anchor to a brand-new key builds a fresh client,
  hands over the store (same wrapper, still initialized, data intact),
  commits the anchor, and retains the old client during its grace
  period.
- Case A init failure: a failing new-client build rolls back — anchor
  pointer stays on the old key, no broken client installed, old client
  preserved, initErr surfaced.
- Case B: re-anchoring onto a key whose client still exists (a former
  anchor in its grace period) reuses the client and builds nothing new
  (clientCh stays empty), while the anchor flips.
- Mobile-primary repoint: switching the primary mobile key to an
  already-accepted mobile key repoints the primary without spawning an
  SDK client — the gap PR #712's gate left, now driven via
  ReconcileResult.MobilePrimaryRepoint.

All four pass under -race.
§7 previously described only Case A (re-anchor onto a brand-new key).
Add the Case B path — re-anchoring onto a key that already has a live
client (typically a former anchor still in its grace period): no
Build, no store handover, just the flip + ReplaceCredential + big-
segment re-wire that Case A also performs after init. The two paths
converge after the flip; the caller branches on c.clients[newAnchor].

Also clarify failure handling (rollback is Case A only; structured
Errorf log, no dedicated alarm infra), the Reconcile/additions
stripping interaction, and update the consolidated T2.c/T2.d
requirements table to reflect the case split and the synchronous
ReplaceCredential call site.
gofmt the rotator's removeCredentialFromList helper, and reword the
reanchorBigSegmentSync stub comment to avoid the godox TODO keyword
(godox is enabled in .golangci.yml; run.tests is false so only
production code is checked). No behavior change.
@aaron-zeisler aaron-zeisler changed the title feat(relayenv): re-anchor mechanism (T2.c, SDK-2542) feat(relayenv): re-anchor mechanism Jun 26, 2026
Re-applies the T2.c deferred-anchor-flip intent on top of the base's refactored Rotator/AcceptedSet API (primarySdkKey->anchorKey, set.primarySdkKey->set.anchor, new SDKKeyParams/MobileKeyParams builder, AcceptedKeys surface):

- rotator.go: Reconcile still returns ReconcileResult and defers the flip to CommitAnchor; removed the in-place anchor flip from reconcileSDKKeys (dropped its now-unused anchor param); Case A still strips the new anchor from additions. Fixed a stale comment referencing the removed legacy RotateWithGrace path.

- rotator_test.go: resolved onto the WithAnchor(SDKKeyParams{...}) builder API; updated TestAcceptedKeys and TestReconcileAlreadyExpiredKeyIsIgnoredOnAdd to the deferred-flip contract (CommitAnchor to observe the anchor; fresh anchor is stripped from additions).

- env_context_reanchor_synchronous_test.go: ported to the new builder API and AnchorKey() accessor.

- env_context_reanchor_test.go: inverted PoC H6/H7 to assert the T2.c fix (no nil swap-window; rollback keeps the old anchor authoritative), completing the same treatment H1/H5 received.
When a synchronous re-anchor's new client fails to initialize, reanchor rolls back and the environment keeps serving from the previous, still-valid anchor. It was also setting c.initErr to the new client's error — but the SDK returns ErrInitializationFailed for an invalid key, and the request middleware treats GetInitError() == ErrInitializationFailed as an env-level auth failure, returning 401 for every request. That rejected all traffic to an environment that was actually healthy, defeating the rollback at the HTTP layer.

Leave initErr untouched on rollback (the env is healthy on the old anchor); the failure still surfaces via the existing structured Error log. Update the Case A init-failure and PoC H7 regression tests to assert GetInitError() stays nil and that the failure is logged at Error level.
The cleanup ticker path (cleanupExpiredCredentials -> triggerCredentialChanges)
drained the rotator's StepTime queue and ran addCredential/removeCredential
without holding reconcileMu, so a credential expiry firing during an in-flight
synchronous re-anchor ran concurrently with the reanchor sequence. Both paths
drain the same StepTime queue (the ticker could steal additions a reconcile just
queued) and removeCredential could close a client mid-reanchor. The synchronous
re-anchor widened this window versus the pre-re-anchor base.

Take reconcileMu for the whole StepTime + add/remove pass in
triggerCredentialChanges so the ticker is serialized against reconcileCredentials
the same way concurrent reconciles already are. reconcileCredentials never calls
that path, so there is no re-entrancy.

Also gate re-anchor peripheral install solely on NewAnchorPreviouslyAccepted
instead of on the Case A/B client-presence branch. The two signals always agree
today (a client can only exist for a previously-accepted key), but branching
peripheral setup on the rotator signal means a brand-new anchor is never stranded
without its routing wiring even if that invariant were ever broken.
Comment thread internal/credential/rotator.go Outdated
Comment thread internal/credential/rotator.go Outdated
Comment thread internal/relayenv/env_context_impl.go Outdated
…rgon from code comments

Addresses PR review comments on comment quality:

- Drop references to the design doc (phase1-design.md, section numbers) and task/epic IDs (T2.x, SDK-25xx, M3) from code comments; those belong in the PR/commit history, not the source.

- Replace 'Case A'/'Case B' in comments with literal descriptions ('the anchor is a new key' / 'a previously-accepted key'), and simplify the re-anchor log strings to drop the same jargon.

- Clarify the reconcileMu comment (it serializes reconcileCredentials calls — only one runs at a time).

- Rephrase PR-number and PoC cross-references to keep the rationale without the identifiers, and fix two stale comments (a removed initErr behavior and the old primarySdkKey field name).
…nchorPeripherals

addCredential and the re-anchor path both need to register a credential's downstream-facing routing — the env-stream registration, per-stream-provider HTTP handlers, and the connection->env mapping. That logic was duplicated: addCredential inlined it and reanchor had a near-verbatim copy in installAnchorPeripherals.

Extract it into registerCredentialMappings(cred) (caller holds c.mu) and call it from both sites, removing installAnchorPeripherals. The name matches the codebase's existing 'credential mappings' vocabulary and replaces the ad-hoc 'peripherals' term throughout the comments.

In addCredential the connection-mapping registration now runs with the other two mappings (before the anchor/mobile switch) instead of after it. This is not observable: the whole function holds c.mu, and the SDK client is started asynchronously (startSDKClient builds the client before taking c.mu), so it installs only after addCredential returns regardless of internal statement order; the three registrations touch disjoint subsystems with no ordering dependency.
Two behavior-preserving simplifications of the re-anchor path:

- Extract buildAndCommitNewAnchorClient from reanchor: the ~45-line new-client build/rollback/install/commit block moves into its own method, leaving reanchor as a short four-way dispatch (register mappings, reuse existing client, offline commit, or build).

- Extract rebuildEvaluatorLocked, shared by startSDKClient and the re-anchor build path, which previously had a near-verbatim copy of the evaluator construction each.

Also clarifies two comments: the reanchor two-signal comment now explains that NewAnchorPreviouslyAccepted (were mappings already registered?) and existingClient (is there already a client?) answer distinct questions and legitimately differ when a previously-accepted non-anchor key is promoted; and restores the reconcileSDKKeys doc comment's opening line, which was dropped during the earlier catch-up merge.
@aaron-zeisler aaron-zeisler marked this pull request as ready for review July 1, 2026 18:55
@aaron-zeisler aaron-zeisler requested a review from a team as a code owner July 1, 2026 18:55
Addresses findings from the multi-agent review of the re-anchor work:

- Guard against a client/store leak when Close() races an in-flight re-anchor build: buildAndCommitNewAnchorClient now re-checks c.closed after re-acquiring c.mu and discards the freshly-built client instead of installing it into a torn-down env (Close does not hold reconcileMu, so it can interleave). Mirrors the existing guard in startSDKClient.

- Clear initErr on all commit paths (reuse/offline as well as build) via commitReanchor, so a stale error from a prior client can't keep a healthy re-anchored env reporting failure to the request middleware.

- store: mark a fully-closed streamUpdatesStoreWrapper and have acquire refuse it, so Build rebuilds a fresh store instead of resurrecting one whose underlying store was torn down.

- Remove the reanchorBigSegmentSync no-op stub; the re-wire hook location is documented at the commit point in commitReanchor for the follow-up.

- Rename rebuildEvaluatorLocked -> rebuildEvaluator (lock requirement stated in the doc); replace removeCredentialFromList with slices.DeleteFunc; drop the last stale 'peripherals' comment.

Adds tests: offline re-anchor, previously-accepted non-anchor promoted to anchor, rotator-level AnchorChange/MobilePrimaryRepoint signalling, and a store rebuild-after-full-close regression.
Comment thread internal/relayenv/env_context_impl.go
A reconcile that both moved the anchor to a new key and immediately revoked the current anchor (no grace expiry) could, on a failed new-client build, still run the expiration phase and close the previous anchor's client — leaving GetClient() nil while the anchor pointer still named that key (503 despite the intended rollback), and leaving the rotator's accepted set naming the failed new key rather than the still-serving anchor.

reanchor now reports whether it committed. On rollback reconcileCredentials backs out just the anchor change: it undoes the failed new anchor's credential mappings, calls the new Rotator.RevertAnchorChange to re-admit the previous anchor and drop the failed new key (realigning the accepted set with the un-moved pointer), and keeps the previous anchor's client serving by not expiring it. Other changes in the same payload still apply. RevertAnchorChange leaves a grace-demoted previous anchor (and its expiry) untouched.

Adds an env-level rollback-with-immediate-revocation test and rotator-level RevertAnchorChange tests for both the revoked and grace-demoted cases.
Comment thread internal/relayenv/env_context_impl.go
…llback

The re-anchor rollback backed out the failed new anchor by unconditionally calling removeCredential on it. For a previously-accepted key promoted to anchor (whose mappings predate the reconcile, so reanchor never re-registered them), that tore down its stream/handler/connection mappings while RevertAnchorChange kept it in the accepted set — leaving the rotator reporting a valid credential the env could no longer serve.

Only undo the new anchor's mappings when it was brand new (NewAnchorPreviouslyAccepted == false), mirroring RevertAnchorChange's accepted-set logic so the env-side and rotator-side undos stay symmetric: a brand-new failed key is fully removed; a previously-accepted one reverts to the non-anchor key it already was, mappings intact.

Adds a regression test (recording connection mapper) asserting a failed promotion of an already-accepted key keeps both its accepted status and its connection mapping.
Comment thread internal/credential/rotator.go
RevertAnchorChange re-admitted change.PreviousAnchor whenever it was missing from the accepted set, without checking it was defined. When an env gains its first SDK key via a reconcile that then fails to build, the previous anchor is the empty (undefined) key, so rollback would insert "" into acceptedSDKKeys — an undefined credential the rotator otherwise never holds. Guard the re-admit on PreviousAnchor.Defined().

Adds a rotator test asserting a rolled-back first-SDK-key reconcile leaves no undefined key in the accepted set.
Comment thread internal/relayenv/env_context_impl.go Outdated
…nterleave

commitReanchor released c.mu after installing the new client and then repointed the anchor/dispatcher, leaving a window where Close() (which does not take reconcileMu) could tear down clients and the dispatcher mid-commit — the commit would then flip to a closed client and call ReplaceCredential on a closed dispatcher.

Split out commitReanchorLocked (caller holds c.mu) and hold a single continuous c.mu across install → commit on the build path; the reuse/offline paths call the self-locking commitReanchor, which now also bails if the env closed. Since Close() takes c.mu, the commit and teardown are now mutually exclusive. This mirrors addCredential, which already repoints event forwarding and reads rotator state under c.mu, so the locking discipline is unchanged. The SDK client is still built outside the lock, so GetClient/GetStore stay live during the build.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 40bfaba. Configure here.

Comment thread internal/relayenv/env_context_impl.go Outdated
Comment thread internal/relayenv/env_context_impl.go
reanchor now acquires c.mu once (defer Unlock) and holds it across the whole sequence — registering credential mappings and committing — releasing it only inside buildAndCommitNewAnchorClient around the SDK client build, which must not hold the lock so GetClient/GetStore stay live. This removes the self-locking commitReanchor wrapper (the commit helper now simply requires the caller to hold c.mu, no Locked suffix) and the separate lock acquisitions reanchor previously made for mapping registration and each commit path.

Because the whole commit runs under one continuous c.mu hold, and Close() also takes c.mu, the commit and teardown are mutually exclusive — closing the remaining windows where Close could tear down the client or dispatcher mid-commit, and making reanchor's committed/rolled-back return value honest on the reuse/offline paths (it now reports the actual commit result, not an unconditional success).
Comment thread internal/relayenv/env_context_impl.go Outdated
buildAndCommitNewAnchorClient unlocked c.mu at its first line and re-locked
it after the build — a helper releasing a lock its caller had taken, which
read awkwardly. Split the pure client build into buildNewAnchorClient, which
holds no lock (it touches only construction-fixed fields, like startSDKClient),
and move the unlock/relock plus install back into reanchor so all of the
lock management lives in one place.

The build still runs outside c.mu: sdkClientFactory can block for up to
sdkInitTimeout, and reconcileMu already lets GetClient/GetStore keep serving
during construction. The reuse-existing-client and offline paths continue to
skip the build entirely; behavior is unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant